feat: superchain erc20 redesign #73

agusduha · 2024-10-01T20:00:51Z

No description provided.

* feat: add superchain erc20 bridge * fix: interfaces and versions

* refactor: use oz upgradeable erc20 as dependency * chore: update interfaces * fix: tests based on changes * refactor: remove op as dependency * feat: add check for supererc20 bridge on modifier * chore: update tests and interfaces * chore: update stack vars name on test * chore: remove empty gitmodules file * chore: update superchain weth errors

* test: add superchain erc20 bridge tests * test: add optimism superchain erc20 beacon tests * test: remove unnecessary test * test: tests fixes * test: tests fixes

* chore: update missing bridge on natspec * fix: natspecs --------- Co-authored-by: agusduha <[email protected]>

--------- Co-authored-by: agusduha <[email protected]>

* refactor: rename mint and burn functions on superchain erc20 * chore: rename optimism superchain erc20 to superchain erc20 * feat: create optimism superchain erc20 contract * chore: update natspec and errors * fix: superchain erc20 tests * refactor: make superchain erc20 abstract * refactor: move storage and erc20 metadata functions to implementation * chore: update interfaces * chore: update superchain erc20 events * fix: tests * fix: natspecs * fix: add semmver lock and snapshots * fix: remove unused imports * fix: natspecs --------- Co-authored-by: 0xDiscotech <[email protected]>

0xParticle

Amazing work guys <3

0xParticle · 2024-10-02T13:58:37Z

packages/contracts-bedrock/src/L2/SuperchainERC20.sol

+    /// @param _to     Address to mint tokens to.
+    /// @param _amount Amount of tokens to mint.
+    function __superchainMint(address _to, uint256 _amount) external virtual onlySuperchainERC20Bridge {
+        if (_to == address(0)) revert ZeroAddress();


whats the reason to enforce this revert in the standard?
Also, if you go this path, why revert here and not in the SuperchainERC20Bridge and save an external call?

0xParticle · 2024-10-02T13:58:49Z

packages/contracts-bedrock/src/L2/SuperchainERC20.sol

+    /// @param _from   Address to burn tokens from.
+    /// @param _amount Amount of tokens to burn.
+    function __superchainBurn(address _from, uint256 _amount) external virtual onlySuperchainERC20Bridge {
+        if (_from == address(0)) revert ZeroAddress();


0xParticle · 2024-10-02T14:02:06Z

packages/contracts-bedrock/src/L2/SuperchainWETH.sol


 /// @title SuperchainWETH
 /// @notice SuperchainWETH is a version of WETH that can be freely transfrered between chains
 ///         within the superchain. SuperchainWETH can be converted into native ETH on chains that
 ///         do not use a custom gas token.
-contract SuperchainWETH is WETH98, ISuperchainERC20Extensions, ISemver {
+contract SuperchainWETH is WETH98, ISuperchainWETH, ISemver {


I was not involved with SuperchainWETH, but it seems like the design has a lot of differences with the SuperchainERC20 now. Can we push for an implementation update here?

0xParticle · 2024-10-02T14:07:01Z

packages/contracts-bedrock/test/L2/SuperchainERC20.t.sol

-    /// @notice Tests the `relayERC20` mints the proper amount and emits the `RelayERC20` event.
-    function testFuzz_relayERC20_succeeds(address _from, address _to, uint256 _amount, uint256 _source) public {
+    /// @notice Tests the `burn` burns the amount and emits the `Burn` event.
+    function testFuzz___superchainBurn_succeeds(address _from, uint256 _amount) public {


you can add the new SuperchainBurn event emision check here. Same for SuperchainMint where it belongs

0xng

Looks good guys!

Run all added/modified tests with the flag --fuzz-runs 100000 if possible so we reduce the possibilities of getting flaky errors.

A general question I have is, we are mentioning L2StandardBridge in many places when the actual contract is called L2StandardBridgeInterop. Should we change it to that, or given that it will most likely be merged into one eventually just leave it at L2StandardBridge. What do you think?

0xng · 2024-10-02T13:39:29Z

packages/contracts-bedrock/src/L2/SuperchainERC20.sol

+    /// @param _to     Address to mint tokens to.
+    /// @param _amount Amount of tokens to mint.
+    function __superchainMint(address _to, uint256 _amount) external virtual onlySuperchainERC20Bridge {
+        if (_to == address(0)) revert ZeroAddress();


not sure how i feel about having this check, it feels opinionated, users calling this can check it themselves if they want, tokens extending it can add it.

also minting happens async, so after the burn, so if the _to is address 0, the user will lose the tokens either way - if anything this check should be in the bridge

0xng · 2024-10-02T13:43:39Z

packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol

+        bytes memory message = abi.encodeCall(this.relayERC20, (_token, msg.sender, _to, _amount));
+        IL2ToL2CrossDomainMessenger(MESSENGER).sendMessage(_chainId, address(this), message);
+
+        emit SendERC20(_token, msg.sender, _to, _amount, _chainId);


Let's stick to past tense like we do for all events for cohesion in the codebase, same in relayERC20. I know these are in present tense in the standard but we should change that

0xng · 2024-10-02T13:46:09Z

packages/contracts-bedrock/src/L2/SuperchainERC20.sol

+    /// @notice Allows the SuperchainERC20Bridge to burn tokens.
+    /// @param _from   Address to burn tokens from.
+    /// @param _amount Amount of tokens to burn.
+    function __superchainBurn(address _from, uint256 _amount) external virtual onlySuperchainERC20Bridge {


the standard says these should be public, we can modify the standard for them to be external also if we prefer it

oh good catch, I rather have them external to enforce the sender check :)

0xng · 2024-10-02T13:48:45Z

packages/contracts-bedrock/src/L2/SuperchainERC20.sol

+    /// @param _from   Address to burn tokens from.
+    /// @param _amount Amount of tokens to burn.
+    function __superchainBurn(address _from, uint256 _amount) external virtual onlySuperchainERC20Bridge {
+        if (_from == address(0)) revert ZeroAddress();


same with this check, not fully convinced it's needed here

fix: update redesign with dev

packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol

+    function sendERC20(address _token, address _to, uint256 _amount, uint256 _chainId) external {
+        if (_to == address(0)) revert ZeroAddress();
+
+        ISuperchainERC20(_token).__superchainBurn(msg.sender, _amount);
+
+        bytes memory message = abi.encodeCall(this.relayERC20, (_token, msg.sender, _to, _amount));
+        IL2ToL2CrossDomainMessenger(MESSENGER).sendMessage(_chainId, address(this), message);
+
+        emit SendERC20(_token, msg.sender, _to, _amount, _chainId);
+    }


0xng

Logic-wise this is approved for me. I am not approved yet because there are some changes I don't get on random files. Similarly, why was forge updated, isn't the version locked?

0xng · 2024-10-03T12:04:05Z

packages/contracts-bedrock/src/L2/interfaces/ISuperchainWETH.sol

@@ -39,8 +39,6 @@ interface ISuperchainWETH {
    /// @param _dst     Address to relay tokens to.
    /// @param _wad     Amount of tokens to relay.
    function relayERC20(address _from, address _dst, uint256 _wad) external;
-
-    function __constructor__() external;


what is this change?

This is smth that was added in develop by the ones doing the OP contracts manager, it was added to many interfaces

This interface was not being used before so now we have to delete that function so that we dont have to implement it

0xng · 2024-10-03T12:04:34Z

packages/contracts-bedrock/scripts/ops/FeeVaultWithdrawal.s.sol

@@ -65,7 +65,7 @@ contract FeeVaultWithdrawal is Script {
    }

    /// @notice Logs the information relevant to the user.
-    function log(uint256 _balance, address _recipient, address _vault) internal pure {
+    function log(uint256 _balance, address _recipient, address _vault) internal view {


why this change?

Compiler error saying that the function uses console.log and it should be view

0xng · 2024-10-03T12:06:32Z

packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol

+        ISuperchainERC20(_token).__superchainBurn(msg.sender, _amount);
+
+        bytes memory message = abi.encodeCall(this.relayERC20, (_token, msg.sender, _to, _amount));
+        IL2ToL2CrossDomainMessenger(MESSENGER).sendMessage(_chainId, address(this), message);


This function now returns the hash of the message, we can debate whether to catch the return value and include this in the event in a following PR.

* fix: semver natspec check failure * fix: ignore mock contracts in semver natspec script * fix: error message

agusduha · 2024-10-03T20:38:40Z

Closing this PR, continuing in #82

agusduha and others added 6 commits September 25, 2024 15:20

feat: add superchain erc20 bridge (#61)

ae024a5

* feat: add superchain erc20 bridge * fix: interfaces and versions

test: add superchain erc20 bridge tests (#65)

6aaf905

* test: add superchain erc20 bridge tests * test: add optimism superchain erc20 beacon tests * test: remove unnecessary test * test: tests fixes * test: tests fixes

chore: update missing bridge on natspec (#69)

11fda67

* chore: update missing bridge on natspec * fix: natspecs --------- Co-authored-by: agusduha <[email protected]>

fix: remove superchain erc20 base (#70)

7a69a7e

refactor: update isuperchainweth (#71)

07c9c77

--------- Co-authored-by: agusduha <[email protected]>

agusduha added the only for review label Oct 1, 2024

agusduha assigned agusduha and 0xDiscotech Oct 1, 2024

agusduha requested review from gotzenx, 0xng, skeletor-spaceman and 0xParticle October 1, 2024 20:44

0xParticle reviewed Oct 2, 2024

View reviewed changes

0xng reviewed Oct 2, 2024

View reviewed changes

agusduha and others added 4 commits October 2, 2024 16:12

fix: refactor zero check (#76)

2e507b2

Merge branch 'develop' into fix/redesign-merge-conflicts

7fcaf6c

fix: pre pr

619f23c

Merge pull request #77 from defi-wonderland/fix/redesign-merge-conflicts

1981789

fix: update redesign with dev

github-advanced-security bot found potential problems Oct 2, 2024

View reviewed changes

0xng reviewed Oct 3, 2024

View reviewed changes

0xng approved these changes Oct 3, 2024

View reviewed changes

fix: semver natspec check failure (#79)

ae019b8

* fix: semver natspec check failure * fix: ignore mock contracts in semver natspec script * fix: error message

agusduha closed this Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: superchain erc20 redesign #73

feat: superchain erc20 redesign #73

agusduha commented Oct 1, 2024

0xParticle left a comment

0xParticle Oct 2, 2024

0xParticle Oct 2, 2024

0xParticle Oct 2, 2024

0xParticle Oct 2, 2024

0xng left a comment

0xng Oct 2, 2024

0xng Oct 2, 2024

0xng Oct 2, 2024

skeletor-spaceman Oct 2, 2024

0xng Oct 2, 2024

0xng left a comment

0xng Oct 3, 2024

agusduha Oct 3, 2024

0xng Oct 3, 2024 •

edited

Loading

agusduha Oct 3, 2024

0xng Oct 3, 2024

agusduha commented Oct 3, 2024

feat: superchain erc20 redesign #73

feat: superchain erc20 redesign #73

Conversation

agusduha commented Oct 1, 2024

0xParticle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xng Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agusduha commented Oct 3, 2024

0xng Oct 3, 2024 •

edited

Loading